-
Notifications
You must be signed in to change notification settings - Fork 15
[ISSUE-7] Added PARALLEL SAFE mark to pgSphere functions #25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Makefile
Outdated
pg_sphere--1.2.1--1.2.2.sql: upgrade_scripts/pg_sphere--1.2.1--1.2.2.sql.in | ||
cat $^ > $@ | ||
ifeq ($(has_parallel), n) | ||
sed -i -e '/PARALLEL/d' $@ # version $(pg_version) does not have support for PARALLEL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking, but shouldn't this line (and lines 120 and 204 and 210) be sed -i -e 's/ *PARALLEL SAFE//' $@
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These lines work only for pg versions less than 9.6 (PARALLEL SAFE is not implemented for earlier versions). Now I removed 9.X versions from tests because these versions are not supported by the community. Furthermore, I'm not sure that sed is executed correctly. It removes the whole line, not only parallel safe marks (IMMUTABLE STRICT seems to be removed as well). When I did the patch I just 'copy-pasted' the old solution. I propose to keep my patch unchanged but to rework these lines in another Issue if we still need support of 9.X versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the sed commands should be sed -i -e 's/ *PARALLEL SAFE//' $@
, but I agree there's no point in continuing to support Pg 9.x, so might as well just remove all of these sed commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@esabol I agree, I think to remove obsolete lines of code. I will do it then.
By the way, if you put "[ISSUE #7]" in the title of the PR instead of "[ISSUE-7]", GitHub will automatically link this PR to that issue. |
The patch is based on Feodor Sigaev's branch (parallel_safe). Some adjustments were made to fix compilation. Checked the number of changes in function declarations and the number of function alterations in the upgrade script pg_sphere--1.2.1--1.2.2.sql.in (the numbers should match). Some stuff in Makefile related to 9.X versions (PARALLEL SAFE removal from install and update scripts) was also removed.
@esabol Thank you for the review! |
The patch is based on Feodor Sigaev's branch (parallel_safe). Some adjustments were made to fix compilation. Checked the number of changes in function declarations and the number of function alterations in the upgrade script pg_sphere--1.2.1--1.2.2.sql.in (the numbers should match).